-
-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
alarm: Long press to stop alarm #2222
base: main
Are you sure you want to change the base?
Conversation
Build size and comparison to main:
|
5290eae
to
1959b3a
Compare
This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.
1959b3a
to
026527d
Compare
I haven't had a chance to test this with hardware since the ringing screen has been redesigned. Does the progress bar need to move at all? Maybe it could go between the time and the button in the blank space. Also if it looks a bit blocky (again I need to test), it will render more nicely if it is thinner since there will be fewer pixels to push. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially with the bigger button, I think this is a good addition. Thanks for the contribution! The only suggestion I have is calculating the bar progress more consistently and making sure it's super clear to the user that they have to hold the button
} | ||
|
||
void Alarm::Refresh() { | ||
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xTaskGetTickCount() - stopBtnPressTime > pdMS_TO_TICKS(150)
is tolerant to tick count wraparound
Should we start showing the bar earlier than 150ms, as you need to hold the button to dismiss it. In fact, would it be better with no delay at all? A long press isn't super intuitive, so giving immediate feedback will make it clearer that holding it is needed IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
xTaskGetTickCount() - stopBtnPressTime > pdMS_TO_TICKS(150)
is tolerant to tick count wraparound Should we start showing the bar earlier than 150ms, as you need to hold the button to dismiss it. In fact, would it be better with no delay at all? A long press isn't super intuitive, so giving immediate feedback will make it clearer that holding it is needed IMO
I think immediately showing the bar makes perfect sense. It makes it responsive and informative about what's going on.
I also think it would make sense that the button itself becomes the indicator. It's currently pretty small, but I think a dedicated "alarm triggered" screen would make sense anyway and would make room for a bigger dismiss button.
Instead of showing the alarm settings screen with a small red button (I don't think it's necessarily that intuitive to show the alarm settings screen when the alarm goes off). My suggestion would be to show the time of the alarm only, as well as a larger dismiss button which will also acts as a long press indicator.
The red background would gradually transition into a new background color. If you let go before it fully transition, it should reset back to full red again.
A larger button will make the progress bar clearly visible even with a thumb on the button. And I think that using the button background as indicator is probably the most intuitive. 🙂 With a bigger dismiss button it could even say "hold to dismiss alarm".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's worth noting the UI has already been changed since #2211 - so the larger button suggestion is already in :)
I like the idea of using the button as the bar, but I've got two concerns: 1) Performance will be poor: as the button is large, rendering will be choppy. 2) The button may be shrunk in the future to accommodate further functionality such a snooze button.
So maybe a thin indicator bar above the button could work? But I'm not sure I love that idea either. Perhaps a progress indicator isn't needed at all, just some feedback that further holding is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first iteration used the button as the bar, but like @mark9064 said: performance was very poor. The second issue was that I could not see the button changing colour (maybe I have fat fingers?) and so there was no feedback whatsoever.
|
||
void Alarm::Refresh() { | ||
if (stopBtnPressing && xTaskGetTickCount() > stopBtnPressTime + pdMS_TO_TICKS(150)) { | ||
stopPosition += 15; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we calculate elapsed ticks with xTaskGetTickCount() - stopBtnPressTime
, I think it'd be better to calculate the bar position using this. So if we want to hold for say 1 second, we can do something like elapsedTicks * 1000 / configTICK_RATE
to get the number of milliseconds the button has been held, and then draw the progress appropriately / stop alerting when more than 1000 milliseconds has passed.
This change prevents accidentally turning off alarm by ensuring that there is a deliberate long press, similar to resetting the Timer app.